fix(reddit): narrow pre-send classification; clarify manual-ad destination by mrautela365 · Pull Request #27 · linuxfoundation/lfx-v2-campaign-service

mrautela365 · 2026-07-14T16:33:51Z

Follow-up to #21 (merged), addressing the two Copilot items that landed after the approval.

Changes

isPreSendDialError classifies ONLY DNS resolution and connect-time dial failures (connection-refused/no-route/network-unreachable) as pre-send (request NOT sent). No TLS error is treated as pre-send (matching the merged Meta client): a TLS error isn't a reliable pre-send proof for an arbitrary caller-supplied transport (renegotiation, or a wrapping RoundTripper surfacing a cert/record error while reading a response after forwarding the POST), so all TLS failures stay AMBIGUOUS (UNCONFIRMED) via transportError. Context cancellation/deadline from an in-flight Do are likewise NOT pre-send — the per-attempt timeout wraps the whole round trip, so a ctx error can fire after the POST reached Reddit. Redirect following is force-disabled on every client used (CheckRedirect → http.ErrUseLastResponse, unconditionally, on a non-mutating shallow copy of a supplied client), which keeps 3xx handling well-defined; a 3xx on a mutating request is UNCONFIRMED.
The manual-ad instruction steps now clarify that the shown click URL carries only the generated utm_* params (the caller's pre-existing query is omitted to avoid persisting a secret) and instruct the operator to set/replace them on their own registration URL (matching buildRedditUTMURL's url.Values.Set, which replaces only the exact utm_* keys it generates and preserves EVERY other query parameter — including an ungenerated utm_* like utm_id; the operator is also told to drop a trailing path slash) — so the manual destination matches an automated ad's.
Added TestIsPreSendDialError (DNS/refused → pre-send; TLS cert/record errors, unexpected-EOF, and Do-time ctx error → ambiguous) and redirect-policy tests (a 3xx on a POST is not followed and surfaces as UNCONFIRMED; a supplied client's CheckRedirect is overridden without mutating the caller's client).

Verified: gofmt, go build, go vet, golangci-lint (0 issues), go test -race ./internal/platform/reddit.

🤖 Generated with Claude Code

cursor · 2026-07-14T16:33:58Z

PR Summary

Medium Risk
Changes how failed mutating creates are classified (TLS, in-flight ctx, mutating 3xx → UNCONFIRMED), which affects retry and reconciliation behavior for paid Reddit resources; logic is heavily tested and documented.

Overview
Hardens paid create outcome classification in the Reddit Ads client so retries are less likely to duplicate campaigns, ad groups, or ads.

Pre-send vs UNCONFIRMED: isPreSendDialError now treats only DNS and connect-time dial failures as definite “not sent.” TLS errors and context cancel/deadline from an in-flight Do are no longer pre-send; they go through transportError and UNCONFIRMED, aligned with the Meta client. Campaign create checks ambiguity before treating caller ctx as a clean pre-POST abort (in-flight cancel during the campaign POST returns partial + UNCONFIRMED).

HTTP behavior: Redirect following is disabled on every client (noFollow / http.ErrUseLastResponse), including WithHTTPClient via a shallow copy. Mutating 3xx responses are classified as ambiguous via isMutatingMethod and createOutcomeAmbiguous.

Operator steps: Manual variant instructions now tell operators to set/replace shown utm_* on their registration URL (matching buildRedditUTMURL), preserve other query params, and drop a trailing path slash; display URLs still omit secrets from steps.

Knowledge docs and broad tests cover classification, redirects, manual variants, and in-flight cancellation.

^{Reviewed by Cursor Bugbot for commit ec3afcb. Bugbot is set up for automated code reviews on this repo. Configure here.}

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 8a36b8a. Configure here.}

Copilot

Pull request overview

Refines Reddit Ads failure classification and manual-ad destination guidance.

Changes:

Classifies TLS and context errors as pre-send failures.
Clarifies manual UTM instructions.
Adds classifier tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`internal/platform/reddit/client.go`	Updates error classification and manual-ad steps.
`internal/platform/reddit/client_test.go`	Adds pre-send classification tests.

…-send isPreSendDialError classified every context.Canceled/DeadlineExceeded from Do as pre-send, but the per-attempt timeout wraps the whole round trip, so a ctx error can fire after the POST reached Reddit. Reporting that as definitely-failed risks a caller double-creating. Route ctx errors to the ambiguous (UNCONFIRMED) path instead, keeping only DNS/dial/TLS-handshake failures as provably pre-send. Add tests for the TLS RecordHeaderError branch and the manual-variant creation path. Addresses Copilot/Cursor review on PR #27. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

dealako

Hi @mrautela365 👋 — nice follow-up. The context-cancellation correction is exactly right: because the per-attempt context.WithTimeout wraps the whole round trip, keeping Do-time ctx errors ambiguous (UNCONFIRMED) instead of pre-send FAILED is the safe call, and the new TestCreateCampaign_CtxCancelDuringCampaignPostIsUnconfirmed (clean under -race, no goroutine leak — the LIFO close(serverDone) before apiSrv.Close() is well done) pins it down. Prior-round Copilot/Cursor items on the ctx branch are resolved in 87447ae.

The TLS branch, however, reintroduces the same misclassification risk on the other side of the round trip, which is why I'm requesting changes.

Revision tracking

✅ Resolved — ctx errors no longer classified pre-send (Cursor High / Copilot); test table flipped; regression test added. (87447ae)
✅ Resolved — tls.RecordHeaderError now covered in TestIsPreSendDialError. (87447ae)
✅ Resolved — manual-variant path now has TestCreateCampaign_ManualVariantsNoPostURL proving the registration-URL secret is stripped. (87447ae) I independently confirmed displayRedditUTMURL rebuilds RawQuery from the allowlist (client.go:1705) and nils userinfo/fragment — secret stripping is correct.
🔴 Still open — TLS pre-send classification is unsound while redirects are followed (Copilot, client.go:664). See inline.

Issue count

🔴 Blocking: 1 — TLS/dial pre-send classification defeated by default redirect-following → double-create risk on a paid resource.
🟡 Minor: 3 — tls.RecordHeaderError is not exclusively pre-handshake; manual-variant "append" guidance diverges from the automated Query.Set semantics; PR description states ctx errors are pre-send, contradicting the implementation.
⚪ Nit: 1 — knowledge-base update convention not followed for a notable outcome-classification fix.

Final decision: 🔴 Needs changes before approval

The redirect/pre-send interaction is the crux — it's cheap to make sound (CheckRedirect = http.ErrUseLastResponse) and directly protects the idempotency guarantee this whole change exists to uphold. Everything else is minor/nit.

Address David's [blocking] and Copilot review: - disable redirect following (CheckRedirect -> ErrUseLastResponse) so a TLS certificate/record error genuinely proves the original POST was unsent; otherwise a POST could be received then redirected to a TLS-broken target and misclassified as not-created, duplicating a paid resource on retry - reword the operator UTM instruction to set/replace the utm_* params (matching buildRedditUTMURL's Query.Set), preserving only other query params - document the outcome-classification fix in the reddit concept doc and log Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

Address Copilot follow-ups on the redirect fix: - drop tls.RecordHeaderError from isPreSendDialError: it can surface after version negotiation while reading a response, so it doesn't prove pre-send; it now flows to the UNCONFIRMED path. CertificateVerificationError stays (handshake-time, provably pre-send) - enforce the no-follow redirect policy on a caller-supplied http.Client too, via a shallow copy so the caller's client is never mutated; an explicit caller CheckRedirect is respected - classify a 3xx response to a mutating request as UNCONFIRMED, since the request reached a responder and a resource may have been committed before the redirect - assert the new operator set/replace UTM instruction text explicitly Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

…(PR #27) Address Copilot follow-ups on the redirect fix: - override CheckRedirect UNCONDITIONALLY, including a caller-supplied client that sets its own callback: a callback that returns nil (or follows N hops then stops) would still follow a redirect and re-open the pre-send hole, so no-follow is a correctness requirement, not a default. The caller's client is copied, never mutated. - restore NewClient's Go doc (the noFollow helper had displaced it) and give noFollow its own doc - correct log.md and the reddit concept doc to match the implementation: tls.RecordHeaderError is excluded from isPreSendDialError (not pre-send), and a 3xx on a mutating request is UNCONFIRMED (not a clean not-created failure) Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

…27) A TLS error is not a reliable pre-send proof for an arbitrary caller-supplied transport: a custom transport can enable renegotiation, and a wrapping/retrying RoundTripper can surface a cert/record error while reading a response after forwarding the POST. Align with the merged Meta client and remove all TLS handling from isPreSendDialError — only DNS and connect-time dial failures now prove pre-send; every TLS failure flows to the UNCONFIRMED path (safe: never lets a retry duplicate a paid resource). Redirect following stays force-disabled so 3xx handling is well-defined, but is no longer relied on to make a TLS error a pre-send proof. Also collapse the duplicated noFollow/NewClient doc comment and align the log + concept doc with the code. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

Follow-up: after removing TLS from isPreSendDialError, several comments and docs still described TLS handshake/cert errors as pre-send. Align them all with the code — only DNS and connect-time dial failures prove pre-send: - createOutcomeAmbiguous doc, the redirect comments, and the TestIsPreSend header now say DNS/dial only; TLS is listed among the ambiguous cases - the concept doc narrows "not proven pre-send" to failures that prove neither pre-send nor rejection, so a definite 4xx is a clean rejection, not UNCONFIRMED Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…ation Follow-up to the merged Reddit Ads client (#21), addressing two Copilot items that landed after David's approval: - client.go: isPreSendDialError now also classifies TLS handshake / certificate failures and context cancellation/deadline as pre-send (request NOT sent), so they are not wrapped as an ambiguous "may exist" transportError. httpClient.Do returning an error does not prove bytes reached Reddit — only a failure after a connection is established and bytes were sent (mid-flight timeout, unexpected EOF) is genuinely ambiguous. - client.go: the manual-ad instruction steps now clarify that the shown click URL carries only the generated utm_* params (the caller's pre-existing query is omitted to avoid persisting a secret), and instruct the operator to append them to their own registration URL — so the manual destination matches an automated ad's. - client_test.go: TestIsPreSendDialError covers DNS/refused/TLS/ctx (pre-send) vs unexpected-EOF (ambiguous). Verified: gofmt, go build, go vet, golangci-lint (0 issues), go test -race ./internal/platform/reddit. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…-send isPreSendDialError classified every context.Canceled/DeadlineExceeded from Do as pre-send, but the per-attempt timeout wraps the whole round trip, so a ctx error can fire after the POST reached Reddit. Reporting that as definitely-failed risks a caller double-creating. Route ctx errors to the ambiguous (UNCONFIRMED) path instead, keeping only DNS/dial/TLS-handshake failures as provably pre-send. Add tests for the TLS RecordHeaderError branch and the manual-variant creation path. Addresses Copilot/Cursor review on PR #27. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Address David's [blocking] and Copilot review: - disable redirect following (CheckRedirect -> ErrUseLastResponse) so a TLS certificate/record error genuinely proves the original POST was unsent; otherwise a POST could be received then redirected to a TLS-broken target and misclassified as not-created, duplicating a paid resource on retry - reword the operator UTM instruction to set/replace the utm_* params (matching buildRedditUTMURL's Query.Set), preserving only other query params - document the outcome-classification fix in the reddit concept doc and log Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Address Copilot follow-ups on the redirect fix: - drop tls.RecordHeaderError from isPreSendDialError: it can surface after version negotiation while reading a response, so it doesn't prove pre-send; it now flows to the UNCONFIRMED path. CertificateVerificationError stays (handshake-time, provably pre-send) - enforce the no-follow redirect policy on a caller-supplied http.Client too, via a shallow copy so the caller's client is never mutated; an explicit caller CheckRedirect is respected - classify a 3xx response to a mutating request as UNCONFIRMED, since the request reached a responder and a resource may have been committed before the redirect - assert the new operator set/replace UTM instruction text explicitly Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…(PR #27) Address Copilot follow-ups on the redirect fix: - override CheckRedirect UNCONDITIONALLY, including a caller-supplied client that sets its own callback: a callback that returns nil (or follows N hops then stops) would still follow a redirect and re-open the pre-send hole, so no-follow is a correctness requirement, not a default. The caller's client is copied, never mutated. - restore NewClient's Go doc (the noFollow helper had displaced it) and give noFollow its own doc - correct log.md and the reddit concept doc to match the implementation: tls.RecordHeaderError is excluded from isPreSendDialError (not pre-send), and a 3xx on a mutating request is UNCONFIRMED (not a clean not-created failure) Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…27) A TLS error is not a reliable pre-send proof for an arbitrary caller-supplied transport: a custom transport can enable renegotiation, and a wrapping/retrying RoundTripper can surface a cert/record error while reading a response after forwarding the POST. Align with the merged Meta client and remove all TLS handling from isPreSendDialError — only DNS and connect-time dial failures now prove pre-send; every TLS failure flows to the UNCONFIRMED path (safe: never lets a retry duplicate a paid resource). Redirect following stays force-disabled so 3xx handling is well-defined, but is no longer relied on to make a TLS error a pre-send proof. Also collapse the duplicated noFollow/NewClient doc comment and align the log + concept doc with the code. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Follow-up: after removing TLS from isPreSendDialError, several comments and docs still described TLS handshake/cert errors as pre-send. Align them all with the code — only DNS and connect-time dial failures prove pre-send: - createOutcomeAmbiguous doc, the redirect comments, and the TestIsPreSend header now say DNS/dial only; TLS is listed among the ambiguous cases - the concept doc narrows "not proven pre-send" to failures that prove neither pre-send nor rejection, so a definite 4xx is a clean rejection, not UNCONFIRMED Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

) - collapse the doubled noFollow doc comment (dangling "Returning" fragment) - correct the createOutcomeAmbiguous comment: a pre-connect DNS/dial failure is pre-send, not a "dial/cert-verification error" - remove the unreachable NOTE after the return in isPreSendDialError (the doc comment already explains the TLS exclusion) - add syscall.ENETUNREACH to the TestIsPreSendDialError preSend table so every branch the function handles is covered Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…* (PR #27) buildRedditUTMURL uses url.Values.Set, which replaces only the exact utm_* keys it generates and preserves every other query parameter — including a foreign utm_* like utm_id the tool doesn't set. The operator instruction wrongly said to keep only non-utm_* params, which would drop utm_id and diverge from the automated destination. Reword to keep ALL other query parameters; update the comment and the test assertion to match. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

#27) - define transportError as "not proven pre-send / may have been sent" (a TLS error routes through it and can fail before OR after bytes are written), not "ALREADY SENT" - add "mutating 3xx" to the createOutcomeAmbiguous contract comments that previously enumerated only transportError/5xx - fix the test comment that still said "keep only non-utm_* params" (all other query params are preserved, including an ungenerated utm_id) - reword the redirect test's rationale to an unreachable DNS/dial target, since TLS errors are no longer classified pre-send Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…27) - the manual-ad instruction now tells the operator to drop a trailing path slash so the manual destination matches buildRedditUTMURL (which strips it) - narrow the log entry: only an in-flight-Do context error is UNCONFIRMED; a cancellation during token refresh is a proven pre-POST failure and stays non-ambiguous Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…g (PR #27) - reword the createOutcomeAmbiguous bullet: transportError is a Do failure NOT PROVEN pre-send (a TLS error can precede connection), not strictly "after a connection was established" - concept doc: a 5xx is returned as apiError and CLASSIFIED by status, not "wrapped"; only transportError-path failures are wrapped Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

The manual-ad instruction test asserted the utm set/replace and query-preservation phrases but not the new "drop any trailing '/'" guidance, so removing it would pass silently and let manual destinations diverge from buildRedditUTMURL again. Add the phrase to the expected list. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

@MRashad26

Add a comment at fetchToken's Do call noting that c.httpClient carries the package-wide no-follow CheckRedirect policy, so a future split of the token client onto its own *http.Client should carry the same policy forward (per @MRashad26). Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Address Copilot review: - client.go:805 (request): reworded the post-Do classification comment — a TLS handshake failure can occur before HTTP bytes are sent, so the comment no longer implies only post-connection failures land in the ambiguous branch; it now says "not proven pre-send / may have been sent" for every non-dial Do error, matching isPreSendDialError. - client.go:1263 (CreateCampaign): removed a stale, contradictory, mid-sentence paragraph that both claimed a caller cancellation is never ambiguous and said to classify ambiguity first; kept a single unambiguous rule (ambiguity-first, with the earlier-step propagation case folded into one sentence). Resolves 2 review threads. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

mrautela365 · 2026-07-17T21:49:50Z

@dealako — thanks for the thorough review. The blocking item (TLS/dial pre-send classification being defeated while redirects are followed → double-create risk on a paid resource) is addressed:

The client now force-disables redirect following via a shared noFollow policy that returns http.ErrUseLastResponse, so a 3xx is handed back to request() and classified (a 3xx on a mutating call is surfaced as UNCONFIRMED) rather than transparently followed to a different target. See internal/platform/reddit/client.go (noFollow, ~L260) and NewClient (~L283–305).
The policy is enforced unconditionally, including on a caller-supplied *http.Client via WithHTTPClient — applied to a shallow copy so the caller's client isn't mutated. This means even a client with its own CheckRedirect can't reintroduce the follow-and-misclassify path.
This landed in commit ec3afcb (post-review).

On the minors: tls.RecordHeaderError handling and the manual-variant guidance were reconciled in the same pass, and the stale comments/PR-description wording were corrected in ec3afcb.

Verified locally on the current HEAD: go build ./..., go vet, go test -race ./internal/platform/reddit/... (pass, no goroutine leak), and golangci-lint run (0 issues). Re-requesting review — thanks!

dealako

Hi @mrautela365 👋 — thanks for grinding through this. That was a long back-and-forth with Copilot on the redirect/TLS classification, and the result is noticeably more rigorous than where we started.

👏 Nice work

The blocking issue from my last round — TLS pre-send classification defeated by default redirect-following — isn't just patched, it's closed from multiple angles: redirects are now force-disabled unconditionally (including on a caller-supplied http.Client via WithHTTPClient, via a non-mutating shallow copy), and every TLS error (not just the redirect-interaction case) was pulled out of the pre-send set entirely once Copilot pointed out that a wrapping/retrying RoundTripper could still surface a TLS-shaped error post-send. That's a more conservative and durable fix than the one I originally suggested.
The new isMutatingMethod + 3xx-on-mutating-method UNCONFIRMED classification closes a follow-on gap (a 3xx now correctly can't be treated as a clean failure) and is symmetric with the pre-existing 5xx handling.
The manual/operator UTM guidance was iterated to actually match buildRedditUTMURL's url.Values.Set semantics (set/replace, preserve all non-generated params including e.g. utm_id, drop a trailing slash) — and each fix is now pinned with an explicit test assertion so the wording can't silently regress again.
Comment/doc hygiene was kept in lock-step with the behavior through every round (transportError, createOutcomeAmbiguous, isPreSendDialError doc comments, docs/knowledge/**), including cleaning up a couple of self-inflicted duplicated-comment-block slips along the way.
TestWithHTTPClientEnforcesNoFollowWithoutMutatingCaller / TestWithHTTPClientOverridesExplicitCheckRedirect are exactly the right tests for the shallow-copy contract — they assert both the enforcement and the non-mutation guarantee.

Revision tracking (since my last review)

✅ Resolved — TLS pre-send classification unsound under redirects (blocking). Redirects force-disabled everywhere; all TLS errors removed from the pre-send set as a second, independent line of defense.
✅ Resolved — tls.RecordHeaderError not exclusively pre-handshake (minor). Moot now — no TLS error is classified pre-send.
✅ Resolved — manual-variant "append" guidance diverging from Query.Set semantics (minor). Reworded to set/replace + preserve all other params + drop trailing slash, with test coverage.
✅ Resolved — PR description contradicting the implemented ctx-error contract (minor). Description now matches code exactly (verified against the current PR body).
✅ Resolved — knowledge-base convention not followed (nit). docs/knowledge/code/internal-platform-reddit.md and docs/knowledge/log.md both updated and kept in sync through every subsequent fix.

I also re-fetched every review thread on the PR via the API — all are marked resolved, and the latest Copilot pass reports no new comments.

New-diff scan (commits since `3be35a7`)

Ran an independent security-auditor pass and a code-reviewer-pro pass over git diff 3be35a7...HEAD in parallel with my own review, plus go test -race ./internal/platform/reddit/... locally (passes clean).

🔴 Blocking: 0
🟡 Minor: 0
⚪ Nit: 1 — the NewClient shallow-copy override (hc := *c.httpClient; hc.CheckRedirect = noFollow) is correct today (no pointer-mutable fields on http.Client get silently shared in a way that matters here), but as a purely stylistic alternative you could construct the copy by naming Transport/Jar/Timeout explicitly instead of a bare struct copy, so intent doesn't depend on http.Client's current field set. Not a defect, take-it-or-leave-it.

Final decision: ✅ Approved

The Reddit client's NewClient value-copied a WithHTTPClient-supplied *http.Client (hc := *c.httpClient) to override CheckRedirect. An http.Client must not be copied after first use — go vet's copylocks analyzer flags it: the struct transitively holds sync primitives, so the copy duplicates its internal mutex while sharing the request-cancellation map, and concurrent use of the caller's client and the copy can race. reddit is the only client on main that enforces no-follow on a caller-supplied client (merged via PR #27), so it is the only one with this value-copy today. The same fresh-client construction is being made to meta (#30) and twitter (#31) as part of ADDING no-follow to those clients. - internal/platform/reddit/client.go: build a fresh *http.Client carrying only the exported reusable fields (Transport, Jar, Timeout) with CheckRedirect: noFollow. The caller's client is neither copied nor mutated. Documented the copylocks rationale and the defensive nil guard. - internal/platform/reddit/client_test.go: assert the fresh client preserves the caller's Transport AND Jar (on top of Timeout / distinct-pointer / caller-not- mutated); reworded stale "copy" test comments to "fresh client". - docs/knowledge: corrected the reddit concept doc's "shallow copy" wording and the log entry (no phantom meta/twitter-on-main precedent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

…ment The Reddit client's NewClient applied its no-follow CheckRedirect to a WithHTTPClient-supplied client by value-copying it (hc := *c.httpClient). This now builds a fresh *http.Client carrying only the caller's documented exported fields (Transport, Jar, Timeout) with CheckRedirect: noFollow. This is a defensive/clarity change, NOT a race fix: on the repo's Go target net/http.Client is exactly those four exported fields with no internal synchronization state, so the old value copy was also correct and go vet's copylocks analyzer does not flag it. The rebuild depends only on the type's public API rather than the struct's internal shape (layout-independent) and won't silently carry over any future unexported field. - internal/platform/reddit/client.go: fresh-client construction with an accurate rationale (no "must not be copied"/race claim); documented the defensive nil guard. - internal/platform/reddit/client_test.go: assert the client used preserves the caller's Transport/Jar/Timeout and the caller is not mutated. The test comment is explicit that it checks observable guarantees, not fresh-vs-copy (both yield a distinct pointer with the same fields). - docs/knowledge: reframed the concept doc + log entry as layout-independence and scoped the log entry to reddit (meta #30 / twitter #31 are separate open PRs). Scope: reddit only — it is the sole client on main enforcing no-follow on a caller-supplied client (merged via PR #27). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>

mrautela365 requested a review from a team as a code owner July 14, 2026 16:33

Copilot AI review requested due to automatic review settings July 14, 2026 16:33

Copilot started reviewing on behalf of mrautela365 July 14, 2026 16:34 View session

cursor Bot reviewed Jul 14, 2026

View reviewed changes

Comment thread internal/platform/reddit/client.go Outdated

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Comment thread internal/platform/reddit/client.go Outdated

Comment thread internal/platform/reddit/client_test.go Outdated

Comment thread internal/platform/reddit/client_test.go Outdated

Comment thread internal/platform/reddit/client.go Outdated

Copilot AI review requested due to automatic review settings July 14, 2026 17:05

Copilot started reviewing on behalf of mrautela365 July 14, 2026 17:06 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Comment thread internal/platform/reddit/client.go

Comment thread internal/platform/reddit/client.go Outdated

Comment thread internal/platform/reddit/client.go Outdated

Comment thread internal/platform/reddit/client.go Outdated

dealako requested changes Jul 14, 2026

View reviewed changes

Comment thread internal/platform/reddit/client.go Outdated

Comment thread internal/platform/reddit/client.go Outdated

Copilot AI review requested due to automatic review settings July 15, 2026 16:46

Copilot started reviewing on behalf of mrautela365 July 15, 2026 16:47 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Comment thread internal/platform/reddit/client.go Outdated

Comment thread internal/platform/reddit/client.go Outdated

Comment thread internal/platform/reddit/client_test.go Outdated

Comment thread internal/platform/reddit/client_test.go

Copilot AI review requested due to automatic review settings July 15, 2026 17:05

Copilot started reviewing on behalf of mrautela365 July 15, 2026 17:05 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 15, 2026 17:12

Copilot started reviewing on behalf of mrautela365 July 15, 2026 17:12 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Comment thread internal/platform/reddit/client.go Outdated

Comment thread internal/platform/reddit/client.go Outdated

Copilot AI review requested due to automatic review settings July 15, 2026 17:28

Copilot started reviewing on behalf of mrautela365 July 15, 2026 17:28 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Comment thread internal/platform/reddit/client.go

Comment thread internal/platform/reddit/client.go

Comment thread internal/platform/reddit/client.go Outdated

Comment thread internal/platform/reddit/client_test.go Outdated

Comment thread docs/knowledge/code/internal-platform-reddit.md Outdated

Copilot AI review requested due to automatic review settings July 15, 2026 17:38

Copilot started reviewing on behalf of mrautela365 July 15, 2026 17:39 View session

mrautela365 added 15 commits July 17, 2026 14:10

mrautela365 force-pushed the fix/reddit-transporterror-narrowing branch from e587090 to ec3afcb Compare July 17, 2026 21:24

Copilot AI review requested due to automatic review settings July 17, 2026 21:24

Copilot started reviewing on behalf of mrautela365 July 17, 2026 21:24 View session

Copilot AI reviewed Jul 17, 2026

View reviewed changes

mrautela365 requested a review from dealako July 17, 2026 21:51

dealako approved these changes Jul 17, 2026

View reviewed changes

dealako merged commit 8d22492 into main Jul 17, 2026
12 checks passed

dealako deleted the fix/reddit-transporterror-narrowing branch July 17, 2026 22:31

This was referenced Jul 18, 2026

fix(platform): disable redirect following on meta and x/twitter clients #30

Merged

fix(platform): build a fresh http.Client in reddit no-follow enforcement #32

Merged

Uh oh!

Conversation

mrautela365 commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

cursor Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dealako left a comment

Choose a reason for hiding this comment

Revision tracking

Issue count

Final decision: 🔴 Needs changes before approval

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mrautela365 commented Jul 17, 2026

Uh oh!

dealako left a comment

Choose a reason for hiding this comment

👏 Nice work

mrautela365 commented Jul 14, 2026 •

edited

Loading

cursor Bot commented Jul 14, 2026 •

edited

Loading

New-diff scan (commits since `3be35a7`)